Add XMLRPC::RackApplication by herwinw · Pull Request #56 · ruby/xmlrpc

herwinw · 2025-04-23T12:00:37Z

This is a simple Rack application that acts as an XMLRPC server. This can be used in any Rack compatible webserver.

There are a few gems that provide a Rack-based XMLRPC server (rack-rpc, xmlrpc-rack_server and threez-rack-rpc), but none of these have been updated in the past 10 years.

Implementation has mostly been copied from ModRubyServer, tests have been inspired by the WebrickServer tests

kou · 2025-04-23T22:20:01Z

test/test_rack_server.rb

+require 'xmlrpc/parser'
+require 'xmlrpc/utils'
+
+module TestXMLRPC


We can remove this because this gem is no longer a default gem. Our test names never conflict with others.

Suggested change

module TestXMLRPC

kou · 2025-04-23T22:20:07Z

test/test_rack_server.rb

+require 'xmlrpc/utils'
+
+module TestXMLRPC
+class Test_Rack < Test::Unit::TestCase


Suggested change

class Test_Rack < Test::Unit::TestCase

class TestRack < Test::Unit::TestCase

kou · 2025-04-24T01:07:26Z

test/test_rack_server.rb

@@ -0,0 +1,79 @@
+# coding: utf-8


We don't need this because it's the default in recent Ruby.

Suggested change

# coding: utf-8

kou · 2025-04-24T01:07:43Z

test/test_rack_server.rb

+
+    s.add_introspection
+
+    return s


Suggested change

return s

s

kou · 2025-04-24T01:10:55Z

test/test_rack_server.rb

+module TestXMLRPC
+class Test_Rack < Test::Unit::TestCase
+  include Rack::Test::Methods
+  include XMLRPC::ParserWriterChooseMixin


Can we use XMLRPC::Create.new(XMLRPC::Config.default_writer.new) and XMLRPC::Config.default_parser.new instead?

kou · 2025-04-24T01:14:35Z

test/test_rack_server.rb

+  def assert_call_success(expect, methodname, *args)
+    request = create().methodCall(methodname, *args)
+    post("/", request, 'CONTENT_TYPE' => 'text/xml')
+    ok, param = parser().parseMethodResponse(last_response.body)
+    assert(ok)
+    assert_equal(expect, param)
+  end


How about using call helper method like the following and assert_equal?

def call(method_name, *args) create = XMLRPC::Create.new(XMLRPC::Config.default_writer.new) request = create.methodCall(method_name, *args) post("/", request, "CONTENT_TYPE" => "text/xml") parser = XMLRPC::Config.default_parser.new parser.parseMethodResponse(last_response.body) end def test_rack assert_equal([true, 9], call("test.add", 4, 5)) assert_equal([false, XMLRPC::FaultException.new(1, "division by zero")], call("test.div", 1, 0)) # ... end

kou · 2025-04-24T01:15:44Z

test/test_rack_server.rb

+  def test_rack
+    # simple call
+    assert_call_success(9, "test.add", 4, 5)
+
+    # fault exception
+    assert_call_failure(1, "division by zero", "test.div", 1, 0)
+
+    # introspection
+    assert_call_success(["test.add", "test.div", "system.listMethods", "system.methodSignature", "system.methodHelp"], "system.listMethods")
+
+    # default handler (missing handler)
+    assert_call_failure(-99, "Method test.nonexisting missing or wrong number of parameters!", "test.nonexisting")
+
+    # default handler (wrong number of arguments)
+    assert_call_failure(-99, "Method test.add missing or wrong number of parameters!", "test.add", 1, 2, 3)
+
+    # multibyte characters
+    assert_call_success("あいうえおかきくけこ", 'test.add', "あいうえお", "かきくけこ")
+  end


Could you create one test per test pattern?

For example:

def test_success assert_call_success(9, "test.add", 4, 5) end def test_exception assert_call_failure(1, "division by zero", "test.div", 1, 0) end

herwinw · 2025-04-24T08:43:13Z

I think I addressed all the issues above. Most of them were to keep the code consistent with the other code/tests. I see this has had a very recent cleanup.

kou · 2025-04-24T08:47:23Z

lib/xmlrpc/server.rb



+# Implements a XML-RPC server, which works with Rack
+class RackServer < BasicServer


In general, Rack server means Puma, Passenger and so on.

Suggested change

class RackServer < BasicServer

class RackApplication < BasicServer

kou · 2025-04-24T08:50:43Z

test/test_rack_server.rb

+  end
+
+  def test_successful_call
+    assert_equal([true, 9], call("test.add", 4, 5))


Could you align arguments for easy to read?

Suggested change

assert_equal([true, 9], call("test.add", 4, 5))

assert_equal([true, 9],

call("test.add", 4, 5))

kou · 2025-04-24T08:51:41Z

test/test_rack_server.rb

+
+    request = create.methodCall(methodname, *args)
+    post("/", request, 'CONTENT_TYPE' => 'text/xml')
+    assert(last_response.ok?)


Could you use power assert for easy to debug? BTW, when does ok? return false?

Suggested change

assert(last_response.ok?)

assert do

last_response.ok?

end

https://github.com/rack/rack/blob/main/lib/rack/response.rb#L189

ok? checks the HTTP status code for 200. This is mostly relevant for the exceptions, where XMLRPC will still return a HTTP OK (200) status, as opposed to using the HTTP status to signal an error like most REST-based APIs do.

This results in output like this:

| | | false #<Rack::MockResponse:0x00007fbc70b25aa0 @original_headers={"Content-type"=>"text/xml; charset=utf-8"}, @errors="", @status=201, @headers={"content-type"=>"text/xml; charset=utf-8", "content-length"=>"296"}, @writer=#<Method: Rack::MockResponse(Rack::Response::Helpers)#append(chunk) /home/herwin/xmlrpc/vendor/bundle/ruby/3.1.0/gems/rack-3.1.13/lib/rack/response.rb:359>, @block=nil, @body=["<?xml version=\"1.0\" ?><methodResponse><fault><value><struct><member><name>faultCode</name><value><i4>-99</i4></value></member><member><name>faultString</name><value><string>Method test.add missing or wrong number of parameters!</string></value></member></struct></value></fault></methodResponse>\n"], @buffered=true, @length=296, @cookies={}> /home/herwin/xmlrpc/test/test_rack_server.rb:73:in `call' /home/herwin/xmlrpc/test/test_rack_server.rb:59:in `test_wrong_number_of_arguments' 56: 57: def test_wrong_number_of_arguments 58: assert_equal([false, XMLRPC::FaultException.new(-99, "Method test.add missing or wrong number of parameters!")], => 59: call("test.add", 1, 2, 3)) 60: end

I don't think this is the most readable output, I think the better option would be something like:

assert(last_response.ok?, "Expected HTTP status code 200, got #{last_response.status} instead")

OK. It's also better than assert(last_response.ok?).

kou · 2025-04-24T08:51:58Z

test/test_rack_server.rb

+  end
+
+  def test_multibyte_characters
+    assert_equal([true, "あいうえおかきくけこ"], call('test.add', "あいうえお", "かきくけこ"))


Suggested change

assert_equal([true, "あいうえおかきくけこ"], call('test.add', "あいうえお", "かきくけこ"))

assert_equal([true, "あいうえおかきくけこ"], call("test.add", "あいうえお", "かきくけこ"))

kou · 2025-04-24T08:52:59Z

lib/xmlrpc/server.rb

+    msg = <<-"MSGEND"
+      <html>
+        <head>
+          <title>#{err}</title>
+        </head>
+        <body>
+          <h1>#{err}</h1>
+          <p>Unexpected error occurred while processing XML-RPC request!</p>
+        </body>
+      </html>
+    MSGEND


Suggested change

msg = <<-"MSGEND"

<html>

<head>

<title>#{err}</title>

</head>

<body>

<h1>#{err}</h1>

<p>Unexpected error occurred while processing XML-RPC request!</p>

</body>

</html>

MSGEND

msg = <<-HTML

<html>

<head>

<title>#{err}</title>

</head>

<body>

<h1>#{err}</h1>

<p>Unexpected error occurred while processing XML-RPC request!</p>

</body>

</html>

HTML

We've got this exact same code snippet two more times in the existing code (CGIServer and ModRubyServer). I would prefer to keep this the same for now, and as a future work extract it to some shared code and remove this duplication. It's easier to find all the occurrences if we keep this exactly the same for now.
Once the code has been deduplicated, this improvement can be applied.

OK. Let's use the approach.

kou · 2025-04-24T08:55:03Z

lib/xmlrpc/server.rb

+    return http_error(400, "Bad Request")        unless parse_content_type(env['CONTENT_TYPE']).first == "text/xml"
+    return http_error(411, "Length Required")    unless length > 0
+
+    req = Rack::Request.new(env)


Do we need to require rack in initialize?

I would guess this is already available when you run this as Rack middleware, so that should only be required if you run this rack app without rack. This seems like an unlikely scenario to me.

OK. Let's revisit this when we receive a bug report / feature request about this.

BTW, this is a Rack application not Rack middleware, right?

This is a simple Rack middleware that acts as an XMLRPC server. This can be used in any Rack compatible webserver.

kou · 2025-04-24T21:17:28Z

Thanks.

And add the missing Rack application to the docs (#56) Closes #57 --------- Co-authored-by: Herwin <herwinw@users.noreply.github.com>

kou reviewed Apr 24, 2025

View reviewed changes

herwinw force-pushed the rack branch 2 times, most recently from a7463e8 to c3f00ce Compare April 24, 2025 08:26

kou reviewed Apr 24, 2025

View reviewed changes

herwinw force-pushed the rack branch from c3f00ce to 37489a6 Compare April 24, 2025 10:03

Add XMLRPC::RackServer

df5c2c3

This is a simple Rack middleware that acts as an XMLRPC server. This can be used in any Rack compatible webserver.

herwinw force-pushed the rack branch from 37489a6 to df5c2c3 Compare April 24, 2025 10:11

herwinw mentioned this pull request Apr 24, 2025

Status ModRubyServer #57

Closed

kou changed the title ~~Add XMLRPC::RackServer~~ Add XMLRPC::RackApplication Apr 24, 2025

kou merged commit 6bf6eb4 into ruby:master Apr 24, 2025
27 checks passed

herwinw deleted the rack branch April 25, 2025 05:38

herwinw mentioned this pull request Apr 25, 2025

Remove ModRubyServer #59

Merged

kou pushed a commit that referenced this pull request Apr 25, 2025

Remove ModRubyServer (#59)

84d4ac2

And add the missing Rack application to the docs (#56) Closes #57 --------- Co-authored-by: Herwin <herwinw@users.noreply.github.com>

	class Test_Rack < Test::Unit::TestCase
	class TestRack < Test::Unit::TestCase



		# Implements a XML-RPC server, which works with Rack
		class RackServer < BasicServer

	class RackServer < BasicServer
	class RackApplication < BasicServer

	assert_equal([true, 9], call("test.add", 4, 5))
	assert_equal([true, 9],
	call("test.add", 4, 5))

	assert_equal([true, "あいうえおかきくけこ"], call('test.add', "あいうえお", "かきくけこ"))
	assert_equal([true, "あいうえおかきくけこ"], call("test.add", "あいうえお", "かきくけこ"))

Conversation

herwinw commented Apr 23, 2025 • edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

herwinw commented Apr 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kou commented Apr 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

herwinw commented Apr 23, 2025 •

edited by kou

Loading